[BUG] Handle optional preamble lines in DepmtxReader and pad csc indptr#534
[BUG] Handle optional preamble lines in DepmtxReader and pad csc indptr#534sarapelka-blyk wants to merge 4 commits intoCORE-GATECH-GROUP:mainfrom
Conversation
[BUG] Handle optional preamble lines in DepmtxReader and pad csc indptr|
Thanks @sarapelka-blyk! And sorry for the delay. I'll get on this this week |
drewejohnson
left a comment
There was a problem hiding this comment.
This behavior would really use some testing. I know the depletion files are lacking in functionality.
Could you create a copy of the existing depletion matrix file and add these preamble / zeros init behavior? Then we can show we get what we expect
serpentTools/parsers/depmatrix.py
Outdated
| # Pad column pointer if file omits trailing all-zero columns | ||
| _, nCols = matrixSize | ||
| indptr = cscProcessor.indptr | ||
| expected = nCols + 1 | ||
| if indptr.shape[0] < expected: | ||
| pad = expected - indptr.shape[0] | ||
| indptr = concatenate([indptr, full(pad, indptr[-1], dtype=indptr.dtype)]) | ||
| elif indptr.shape[0] > expected: | ||
| raise ValueError(f"index pointer size ({indptr.shape[0]}) should be ({expected})") | ||
| self.depmtx = csc_matrix( | ||
| (cscProcessor.data[:, 0], cscProcessor.indices, | ||
| cscProcessor.indptr), dtype=longfloat, shape=matrixSize) | ||
| (cscProcessor.data[:, 0], cscProcessor.indices, indptr), | ||
| dtype=longfloat, shape=matrixSize) |
There was a problem hiding this comment.
This logic isn't immediately clear to me. If the shape of the matrix is known ahead of construction, does it matter that there are full zero columns? They shouldn't have any information in the sparse matrix structure.
Can you add some testing for this logic?
There was a problem hiding this comment.
When I ran it originally I got an error. As I understand it, scipy.sparse.csc_matrix expects indptr to be length n_cols + 1, even when some columns are all zero. In the depmtx files the matrix size includes columns that never appear in the A matrix, so those columns are implicit zeros. Padding indptr just makes the sparse structure consistent with the declared shape and avoids the pointer-size error.
serpentTools/parsers/depmatrix.py
Outdated
| BaseReader.__init__(self, filePath, 'depmtx') | ||
| SparseReaderMixin.__init__(self, sparse) | ||
| self.deltaT = None | ||
| self.flx = None |
There was a problem hiding this comment.
Similar comments from #512
- Please add this to the
Attributessection of the docstring, when it may exist or not, what purpose it serves, etc. - Please consider using the full name
self.flux
|
My apologies, for taking a while! I have not forgotten about this, but I have been overwhelmed with work. I expect to be able to get back to this in December. |
drewejohnson
left a comment
There was a problem hiding this comment.
Thanks for the updates @sarapelka-blyk! I have some minor comments and then this will be ready to rock
tests/test_depmtx.py
Outdated
| # optional flux captured | ||
| if hasattr(self.reader, "flx"): | ||
| self.assertIsInstance(self.reader.flx, float) |
There was a problem hiding this comment.
The attribute is now named flx so this will need an update. If we just want to make sure we have something, we can instead use
| # optional flux captured | |
| if hasattr(self.reader, "flx"): | |
| self.assertIsInstance(self.reader.flx, float) | |
| # optional flux captured | |
| self.assertIsNotNone(self.reader.flux) |
tests/test_depmtx.py
Outdated
| self.assertAlmostEqual(A[1, 1], 2.0) # A(2,2) | ||
| self.assertAlmostEqual(A[2, 1], -2.0) # A(3,2) | ||
|
|
||
| def test_sparse_indptr_invariants_if_sparse(self): |
There was a problem hiding this comment.
I would greatly appreciate a docstring here as to what this test is accomplishing. At a certain point, it feels like we're testing the behavior of the scipy sparse arrays and I'm not sure how much value that adds.
But, the value is we want to make sure the indptr stops advancing for the last two columns. That's captured in the comments, but I would like it emphasized and stated in the docstring
There was a problem hiding this comment.
added blurb, let me know if you were thinking of something more detailed
tests/test_depmtx.py
Outdated
| fd, cls._tmp = tempfile.mkstemp(suffix=".m", prefix="depmtx_optional_") | ||
| os.close(fd) | ||
| with open(cls._tmp, "w", encoding="utf-8") as f: | ||
| f.write(content) | ||
|
|
There was a problem hiding this comment.
Hmm would using a NamedTemporaryFile be easier? Then we don't need to worry about the file descriptor object nor calls to os.close. Something like
@classmethod
def setUpClass(cls):
content = """..."""
cls._tmp = tempfile.NamedTemporaryFile(mode="w")
cls._tmp.write(content)
# Set file pointer back to start of file so we read from the top
cls._tmp.seek(0)
...
@classmethod
def tearDownClass(cls):
cls._tmp.close()This may still require the try/except pattern during removal but I'm not sure.
There was a problem hiding this comment.
changed. because i'm on windows i have set the delete flag to False (per https://docs.python.org/3/library/tempfile.html ), but that should be okay with linux and macs?
tests/test_depmtx.py
Outdated
| """Base that writes a tiny depmtx file with optional preambles present.""" | ||
|
|
||
| USE_SPARSE = True | ||
| __test__ = False |
There was a problem hiding this comment.
Hmm this is new to me!1
Since Pytest 2.6, users can prevent pytest from discovering classes that start with
Testby setting a boolean__test__attribute toFalse.
Looking at the two concrete classes below, could we have the "base" class be SparseOptionalPreambleTester and a subclass for dense? The only difference in the two is handling of the sparsity. Then we have two classes and don't need to modify the __test__ attribute
Footnotes
drewejohnson
left a comment
There was a problem hiding this comment.
Thanks @sarapelka-blyk!
|
Hmm looks like our CI is very outdated python runs: 3.6-3.9 I'll take a look hopefully soon and get that resolved elsewhere. Nothing for you to do, this can be considered complete 🚀 |
Aha! The target branch is If you can rebase your branch off |
Base branch changed by a user without write access
The base branch was changed.
d94632b to
ab22e57
Compare
This was an exercise in patience and taking slow breaths. |
|
all tests pass on my machine :((( i will look into the problem |
|
Not sure what had happened, but somewhere along the way I must have made a file binary instead of a text file. It should be fixed now. |
|
I cannot recreate the failing tests because they are passing locally :( |
|
I haven't forgotten about this issue again I promise! I will need to dedicate some time to it when I am available, hopefully within a month! |
No worries, @sarapelka-blyk! I have not been very responsive, and I don't love that. You know how to reach me if you want to brain storm and/or if this becomes urgent. Here to help :) |
Closes #533
Make sure you have read over the developer guide to ease
the review process. These include:
docs/contributors.rstInclude a thorough and concise overview of the changes, and why this PR is overall beneficial to the project.
The previous version of the reader errors out when it comes across a preamble/initialization of the N0, N1, A, and Z matrices. Additionally it cannot handle a normalization factor at the top of the file.
Changes made:
Handle optional preamble lines in DepmtxReader (flx, N0/ZAI/N1)
flx = ...normalization factorN0 = zeros(...),ZAI = zeros(...),N1 = zeros(...)initializersindptrwhen depmtx has trailing all-zero columnsDepmtxReader.flx